Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[EXPORTER and SDK] Additional fixes after NOMINMAX removal on Windows #2475

Merged
merged 3 commits into from
Jan 3, 2024

Conversation

meastp
Copy link
Contributor

@meastp meastp commented Jan 3, 2024

Fixes build failures after removal of NOMINMAX - previous commits from @ThomsonTan didn't have full coverage.

@meastp meastp requested a review from a team January 3, 2024 14:18
Copy link

linux-foundation-easycla bot commented Jan 3, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@owent owent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, and LGTM after CLA signed.

Copy link
Member

@lalitb lalitb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once CLA is signed.

@meastp meastp force-pushed the meastp-fix-nominmax branch from 052d266 to f02636d Compare January 3, 2024 20:32
@meastp
Copy link
Contributor Author

meastp commented Jan 3, 2024

I modified my email, CLA is signed. Should be ready to merge :)

Thanks, @ThomsonTan @lalitb @owent

Copy link

codecov bot commented Jan 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ddfafff) 87.04% compared to head (f02636d) 87.06%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2475      +/-   ##
==========================================
+ Coverage   87.04%   87.06%   +0.02%     
==========================================
  Files         199      199              
  Lines        6087     6087              
==========================================
+ Hits         5298     5299       +1     
+ Misses        789      788       -1     

see 1 file with indirect coverage changes

@lalitb lalitb merged commit 0134bbe into open-telemetry:main Jan 3, 2024
46 checks passed
waywardmonkeys added a commit to waywardmonkeys/vcpkg that referenced this pull request Jan 4, 2024
The patch to add a dependency of opentelemetry-proto on gRPC should
not be needed after the upstream changes in:

open-telemetry/opentelemetry-cpp#2268

Additionally, the "zpages" feature is removed as it is no longer
present upstream after having been deprecated in a previous release.

A patch is added extracted from open-telemetry/opentelemetry-cpp#2475
which fixes problems that arose after `NOMINMAX` was no longer
defined within opentelemetry-cpp on Windows.

Fixes microsoft#35992.
waywardmonkeys added a commit to waywardmonkeys/vcpkg that referenced this pull request Jan 4, 2024
The patch to add a dependency of opentelemetry-proto on gRPC should
not be needed after the upstream changes in:

open-telemetry/opentelemetry-cpp#2268

Additionally, the "zpages" feature is removed as it is no longer
present upstream after having been deprecated in a previous release.

A patch is added extracted from open-telemetry/opentelemetry-cpp#2475
and open-telemetry/opentelemetry-cpp#2449
which fix problems that arose after `NOMINMAX` was no longer
defined within opentelemetry-cpp on Windows.

Fixes microsoft#35992.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants